percpu reg store this_task #13726

hujun260 · 2024-09-29T09:40:55Z

Summary

We can utilize percpu storage to hold information about the
current running task. If we intend to implement this feature, we would
need to define two macros that help us manage this percpu information
effectively.

up_this_task: This macro is designed to read the contents of the percpu
register to retrieve information about the current
running task.This allows us to quickly access
task-specific data without having to disable interrupts,
access global variables and obtain the current cpu index.

up_update_task: This macro is responsible for updating the contents of
the percpu register.It is typically called during
initialization or when a context switch occurs to ensure
that the percpu register reflects the information of the
newly running task.

Impact

arm64

Testing

Configuring NuttX and compile:
$ ./tools/configure.sh -l qemu-armv8a:nsh_smp
$ make
Running with qemu
$ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic \
   -machine virt,virtualization=on,gic-version=3 \
   -net none -chardev stdio,id=con,mux=on -serial chardev:con \
   -mon chardev=con,mode=readline -kernel ./nuttx

nuttxpr · 2024-09-29T09:45:50Z

[Experimental Bot, please feedback here]

The provided PR description is insufficient based on the NuttX requirements.

Here's why and how to improve it:

Missing Information

Summary:
- Why is this change necessary? What specific problems does using percpu storage for task information solve? Is there a performance bottleneck this addresses? Is it a prerequisite for another feature?
- What functional part of the code is being changed? Be more specific than "arm64". Are you modifying the scheduler? A specific architecture port?
Impact:
- Is this a new feature or changing an existing one? This is important for understanding the scope.
- Impact on user: Even if you believe the impact on the user is minimal, state that explicitly. If there's no impact on the user API, say so.
- Impact on build: Will any new configuration options be added?
- You only mention "arm64". Will this impact other architectures? If so, how? If not, state that this is arm64 specific.
- Impact on documentation: New features or significant changes require documentation updates. Indicate if you are providing documentation updates as part of this PR.
- Security, compatibility: Even if there is no impact, state this explicitly.
Testing:
- Insufficient build host details: Specify the operating system (e.g., Ubuntu 20.04), CPU architecture (x86_64), and compiler version used for testing.
- "Running with qemu" is not enough:
  - Provide the full QEMU command line used.
  - Crucially, you need to show HOW you tested this. What specific commands did you run in the NuttX shell (nsh) to validate that up_this_task and up_update_task work as expected?
- Show both "before" and "after" logs. What is different in the output that demonstrates the change is working?

Recommendations

Provide concrete motivations for the change.
Be specific about the code being modified.
Explain the user impact even if it is minimal.
Detail build system changes.
Clarify architecture specificity (is it only arm64?).
Describe any new or modified documentation.
Provide comprehensive test cases with expected output. Demonstrate how you validated that the percpu storage is being used correctly.

By addressing these points, your PR will be much clearer and easier for the NuttX maintainers to review.

sched/irq/irq_csection.c

include/nuttx/arch.h

sched/sched/sched.h

arch/arm64/include/arch.h

arch/arm64/include/irq.h

arch/arm64/src/common/arm64_doirq.c

pussuw · 2024-10-02T08:52:47Z

arch/arm64/src/common/arm64_cpustart.c

@@ -226,6 +222,12 @@ int up_cpu_start(int cpu)

 void arm64_boot_secondary_c_routine(void)
 {
+  struct tcb_s *tcb = current_task(this_cpu());


I think most places use this_task(), does it not work here ?

it not work here,
Because we need to complete the initialization percpu before using percpu, and this is where the initialization takes place.

pussuw

LGTM

xiaoxiang781216 · 2024-10-08T18:41:20Z

arch/arm64/include/irq.h

-{
-  __asm__ volatile ("msr " "tpidr_el1" ", %0" : : "r" (regs));
-}
+#define up_current_regs()      (this_task()->xcp.regs)


let's remove up_current_regs from common code

reason: We can utilize percpu storage to hold information about the current running task. If we intend to implement this feature, we would need to define two macros that help us manage this percpu information effectively. up_this_task: This macro is designed to read the contents of the percpu register to retrieve information about the current running task.This allows us to quickly access task-specific data without having to disable interrupts, access global variables and obtain the current cpu index. up_update_task: This macro is responsible for updating the contents of the percpu register.It is typically called during initialization or when a context switch occurs to ensure that the percpu register reflects the information of the newly running task. Configuring NuttX and compile: $ ./tools/configure.sh -l qemu-armv8a:nsh_smp $ make Running with qemu $ qemu-system-aarch64 -cpu cortex-a53 -smp 4 -nographic \ -machine virt,virtualization=on,gic-version=3 \ -net none -chardev stdio,id=con,mux=on -serial chardev:con \ -mon chardev=con,mode=readline -kernel ./nuttx Signed-off-by: hujun5 <[email protected]>

Signed-off-by: hujun5 <[email protected]>

…task more earlier. A call stack looks like the following: sched_idletask syslog_write nx_vsyslog syslog getreg64 gic_validate_redist_version arm64_gic_init arm64_gic_secondary_init arm64_boot_secondary_c_routine Signed-off-by: hujun5 <[email protected]>

GUIDINGLI · 2024-10-15T07:25:52Z

LGTM

github-actions bot added Area: Networking Effects networking subsystem Arch: arm64 Issues related to ARM64 (64-bit) architecture Area: File System File System issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Sep 29, 2024